-
Notifications
You must be signed in to change notification settings - Fork 209
feat: Add stream_workspace resource + ds to replace stream_instance #3832
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
2c62840 to
19286c8
Compare
|
Please review by individual commit to filter out all the docs updates. My thought was that we can take an initial approach to convert the stream workspace model to the stream instance model and back to reduce code duplication. Happy to take a different approach if the team does not feel comfortable with this approach As a follow-up to this PR we can consider refactoring this approach, but for now I felt it provides functionality without making the PR too complex. I mostly copied over a lot of the stream instance code to achieve the addition of the new resource and datasource |
|
APIx bot: a message has been sent to Docs Slack channel |
xargom
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
kpatel71716
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added logical commits to refactor the docs, simplify var naming that was copy/pasted over + remove copied unused methods, and refactored the workspace/instance struct mapping logic for a better intermediate solution as we transition
| @@ -0,0 +1,175 @@ | |||
| package streamworkspace | |||
|
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filenames should not have the resource name. New convention similar to streamprocessor instead (remove stream_workspace from the name)
3247211 to
b833d7f
Compare
EspenAlbert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! Thank you for addressing the comments 👏
| @@ -1,10 +1,20 @@ | |||
| # DEPRECATED: This example uses the deprecated mongodbatlas_stream_instance resource. | |||
| # For new deployments, use mongodbatlas_stream_workspace instead. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider removing examples entirely for deprecated resources to avoid customer confusion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, done! I have CLOUDP-347566 to do this more extensively as a quick follow-up to reduce noise in this PR
|
|
||
| func ResourceSchema(ctx context.Context) schema.Schema { | ||
| return schema.Schema{ | ||
| DeprecationMessage: fmt.Sprintf(constant.DeprecationNextMajorWithReplacementGuide, "resource", "mongodbatlas_stream_workspace", "https://registry.terraform.io/providers/mongodb/mongodbatlas/latest/docs/guides/stream-instance-to-stream-workspace-migraton-guide"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: consider fixing the docs version in the URL to protect from guide url renames in future versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, added this as part of CLOUDP-347566 as a quick follow-up
| "go.mongodb.org/atlas-sdk/v20250312009/admin" | ||
| ) | ||
|
|
||
| // newStreamWorkspaceCreateReq creates an API request for creating a stream workspace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // newStreamWorkspaceCreateReq creates an API request for creating a stream workspace | |
| // newStreamWorkspaceCreateReq creates an API request for creating a stream workspace. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done on this and other comments in code!
| if resp.Diagnostics.HasError() { | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: my preference is not to have so many empty lines, similar for other functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, reduced the new lines here
|
Failing test unrelated and we have been investigating and fixing some flakiness. Merging |
Description
CLOUDP-335390 As part of the effort to rename Atlas Stream Processing Instances to Workspaces, we are adding a new stream_workspace resource/datasource to replace stream_instance
Link to any related issue(s):
Type of change:
Required Checklist:
Further comments